Refactor variable conversion into stage-independent common code - #1679
Refactor variable conversion into stage-independent common code#1679sbryngelson wants to merge 19 commits into
Conversation
Resolves conflicts with MFlowCode#1675 (hyperelasticity removal): - accept deletion of src/simulation/m_hyperelastic.fpp - adopt hypoelasticity in place of the removed elasticity flag - drop GPU_DECLARE for elasticity, b_size, tensor_size - drop hyperelasticity from SIM_GPU_DECL_VARS, whose parameter master removed (silent auto-merge break; failed the generator test)
Collapse the optional-argument dispatch in s_convert_species_to_mixture_variables: absent optional dummies forward as absent, so the four branches were unnecessary. Default dirichlet_from_buffers so it is defined before module initialization. Fail generation when SIM_GPU_DECL_VARS names a variable simulation does not declare, instead of silently dropping its device residency. Drop a stray build log and refresh the stage-independence notes.
COMMON_CASE_OPT_EXTRA_NAMES silently emitted nothing for a name that CASE_OPT_EXTRA_LINES does not define, leaving pre_process and post_process without a declaration that common code references. Raise instead, matching the SIM_GPU_DECL_VARS guard.
There was a problem hiding this comment.
Pull request overview
This PR is an incremental refactor to make src/common/ genuinely stage-independent by removing MFC_SIMULATION/MFC_PRE_PROCESS/MFC_POST_PROCESS-based implementation selection and replacing it with explicit runtime policies and explicit data ownership/arguments. It also updates the parameter generator to separate “declared in Fortran” from “accepted in a stage namelist” so common code can compile for all executables without expanding user-facing inputs.
Changes:
- Update the Fortran parameter generator to support declaration targets independent of namelist targets, and to centralize simulation scalar GPU residency via
SIM_GPU_DECL_VARS. - Make common modules stage-independent via explicit policies/arguments (variables conversion, boundary/grid buffering, decomposition, MPI I/O binding, Dirichlet behavior, finite differences, STL setup, phase change).
- Rename scalar minimum spacings to
dx_min/dy_min/dz_minand propagate usage across pre_process and common IC/model logic.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| toolchain/mfc/params/generators/fortran_gen.py | Adds declaration-target support and simulation GPU_DECLARE ownership enforcement. |
| toolchain/mfc/params/definitions.py | Introduces DECLARATION_TARGETS to decouple Fortran visibility from namelist acceptance. |
| toolchain/mfc/params_tests/test_fortran_gen.py | Adds tests ensuring declarations don’t expand namelists and that GPU_DECLARE ownership rules hold. |
| src/simulation/m_start_up.fpp | Passes explicit common policies (checker, MPI common, variables conversion, boundary common, decomposition) and explicit grid-buffer population. |
| src/simulation/m_sim_helpers.fpp | Switches to the shared species→mixture conversion kernel. |
| src/simulation/m_ibm.fpp | Switches to the shared species→mixture conversion kernel. |
| src/simulation/m_global_parameters.fpp | Removes sim-local ownership of arrays moved to common; updates initialization policy calls. |
| src/simulation/m_data_output.fpp | Updates MPI I/O initialization calls to new explicit interfaces (includes downsample path). |
| src/simulation/m_cbc.fpp | Switches to the shared species→mixture conversion kernel. |
| src/simulation/m_bubbles_EL.fpp | Switches to the shared species→mixture conversion kernel. |
| src/pre_process/m_start_up.fpp | Uses explicit common checker/decomposition policies and renames min spacings to *_min. |
| src/pre_process/m_icpp_patches.fpp | Updates smoothing logic to use dx_min/dy_min/dz_min. |
| src/pre_process/m_grid.f90 | Renames grid-spacing scalar usage to dx_min/dy_min/dz_min and updates calculations accordingly. |
| src/pre_process/m_global_parameters.fpp | Removes pre-process scalar dx/dy/dz declarations and redundant defaults now owned in common. |
| src/pre_process/m_data_output.fpp | Updates downsample MPI I/O initialization and explicit QBMM binding in MPI I/O. |
| src/post_process/m_start_up.fpp | Uses explicit common checker/decomposition policies and explicit grid-buffer population. |
| src/post_process/m_global_parameters.fpp | Adjusts defaults now owned in common and updates eqn index init policy. |
| src/post_process/m_data_input.f90 | Updates MPI I/O initialization for the new explicit IB descriptor interface and downsample signature. |
| src/common/m_variables_conversion.fpp | Removes stage selection, introduces explicit conversion policies, and shares one host/device conversion kernel. |
| src/common/m_phase_change.fpp | Enables the full phase-change module to compile for all executables. |
| src/common/m_mpi_common.fpp | Makes MPI I/O binding and transport/decomposition stage-independent with explicit policies and interfaces. |
| src/common/m_model.fpp | Makes STL instantiation available in all executables and switches to *_min spacings. |
| src/common/m_global_parameters_common.fpp | Centralizes shared declarations, adds dx_min/dy_min/dz_min, and replaces stage guards with explicit init policies. |
| src/common/m_finite_differences.fpp | Removes post-process-only allocation side effects from shared FD coefficient routines. |
| src/common/m_checker_common.fpp | Replaces stage-based total-cell checking with explicit caller policy and explicit n_global argument. |
| src/common/m_boundary_primitives.fpp | Makes Dirichlet ghost-filling behavior a runtime policy via a device-resident flag. |
| src/common/m_boundary_common.fpp | Refactors grid-buffer population to an explicit-array interface shared across dimensions and stages. |
| src/common/include/macros.fpp | Makes unified-memory preference depend on capability rather than stage. |
| src/common/include/3dHardcodedIC.fpp | Updates hardcoded IC logic to use dx_min. |
| src/common/include/2dHardcodedIC.fpp | Updates hardcoded IC logic to use dx_min/dy_min. |
| .claude/rules/common-pitfalls.md | Updates documented “common pitfalls” guidance to match the new stage-independent pattern and generator ownership. |
Comments suppressed due to low confidence (1)
src/simulation/m_data_output.fpp:718
- The unconditional
s_initialize_mpi_data(q_cons_vf, ...)call overwrites any downsample-specific binding ofMPI_IO_DATA%varestablished earlier, which can cause downsampled writes (especially in thebubbles_eulerpath) to use full-resolution buffers. Guard this call so it does not run whendown_sampleis active.
call s_initialize_mpi_data(q_cons_vf, qbmm_pb=pb_ts(1), qbmm_mv=mv_ts(1))
riemann_solver is not a case-optimization parameter, so its default belongs outside the case-optimization guard; case-optimized simulation builds otherwise lost the dflt_int sentinel. post_process declares viscous for the common conversion kernel but neither accepts it as input nor broadcasts it, and assigns defaults on rank 0 only, so give it a value on every rank. Initialize the MPI transport policies, keep global_bounds intent(inout) so inactive dimensions retain their defaults, and mark the grid-width buffer contiguous where an element is passed to MPI.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1679 +/- ##
==========================================
- Coverage 61.04% 60.77% -0.28%
==========================================
Files 83 83
Lines 20978 20872 -106
Branches 3099 3101 +2
==========================================
- Hits 12807 12685 -122
+ Misses 6126 6121 -5
- Partials 2045 2066 +21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…NaNs) CCE 19.0.0 silently drops intent(inout) stores to a non-contiguous assumed-shape array dummy when the same call also passes a contiguous one. s_mpi_sendrecv_grid_variable_buffer marked only cell_widths contiguous, so the ghost values of x_cb and x_cc were never written at rank seams. s_initialize_weno_module then builds reconstruction coefficients from x_cb across that ghost range, with x_cb differences in the denominators, so every multi-rank case grew a NaN within ~50 steps. Multi-rank only because the routine runs only for bc_edge >= 0. Cray GPU only because CMakeLists applies -Oipa0 to the whole simulation target on CPU but to just four files on GPU, and the caller m_boundary_common is not one of them; the failure tracks the caller's IPA level. Declaring every array dummy on the chain contiguous avoids the mixed-attribute call. All actual arguments are whole module allocatables, so the attribute is accurate. Verified on Frontier with CCE 19.0.0 and OpenMP offload: all 8 multi-rank tests pass (0FCCE9F1, 8C7AA13B, CE232828, AFBACA70, CB0DC420, 75D7CC39, 0090B316, 0DDE8A87), previously all NaN.
…owdown) Moving dir_idx/dir_flg/dir_idx_tau from simulation's m_global_parameters into m_global_parameters_common cost ~18 kB/work-item of register spill in every dimensionally-split kernel that reads them under AMD OpenMP offload. m_riemann_solver_hll.fpp, whose source this PR never touches, went 828 B -> 18832 B of scratch, VGPR 72 -> 128 (ceiling), 100 ms -> 1430 ms per direction - about 100% of the regression. s_convert_primitive_to_flux_variables went 60 B -> 17888 B. Kernels that never read the mapping (weno, 28 B both ways) or that were already VGPR-saturated on master (s_convert_conservative_to_primitive_variables, 18272 B both ways) were unaffected: the slowdown tracks dir_idx/dir_flg reference count exactly - 45 refs 14x, 10 refs 11x, 0 refs 1.00x. Declare the mapping in simulation as on master and pass it explicitly into the single common consumer, s_convert_primitive_to_flux_variables (called only from m_cbc). src/common keeps zero stage guards and now holds no reference to simulation direction state. Measured on hpcfund gfx90a (MI250X), amdflang AFAR 23.2.0, OpenMP offload, 1 rank, benchmarks/5eq_rk3_weno3_hll --gbpp 4, rocprofv3 kernel trace: grind ns/gp/eq/rhs master 3.128 | PR 7.460 | PR+fix 3.068; total device time 1995 | 5969 | 1951 ms; Riemann scratch 828 | 18832 | 828 B. Local suite was 186/627 with 0 failures at push time; full run and CCE coverage via CI.
… NVHPC build) copyinReadOnly emits OpenACC copyin(readonly: ...); nvfortran rejects it with NVFORTRAN-S-0034-Syntax error at or near : and the gpu-acc lanes failed to build m_variables_conversion. The OpenMP path emits map(to: ...) for both spellings, so the amdflang build I validated on could not see it. copyin is the spelling already used in-tree (m_time_steppers, m_body_forces); no other call site uses copyinReadOnly on a parallel loop. Verified by generating the directives with fypp: !$acc& copyin(dir_idx_in, dir_flg_in) and !$omp& map(to:dir_idx_in, dir_flg_in). The OMP text is unchanged, so the measured 3x recovery from 89c25e3 stands.
Description
This contains the first incremental steps toward making
src/commongenuinely stage-independent instead of selecting different implementations withMFC_SIMULATION,MFC_PRE_PROCESS, andMFC_POST_PROCESS.The first slice focuses on
m_variables_conversion:Re_idx,Re_size, material arrays, and the new policy scalars.The second slice makes shared grid-buffer population stage-independent:
m_boundary_commonnow receives cell-boundary, cell-center, and cell-width arrays explicitly instead of selecting stage-global representations;buff_sizecell-boundary extent, while post-process supplies its asymmetric Silo/output offsets;glb_boundsand performs the GPU update immediately after the host-side grid population.The third slice removes the superseded grid implementations and separates minimum spacing from width-array storage:
dx_min,dy_min, anddz_minscalars;dx,dy, anddzwidth arrays unchanged;The fourth slice makes MPI I/O field binding and downsampled datatype construction stage-independent:
pb/mvorpb_ts(1)/mv_ts(1)by stage;m_mpi_common, reducing the remaining stage-condition count insrc/commonfrom 30 to 24.The fifth slice makes Dirichlet ghost filling an explicit runtime policy:
s_dirichlet;GPU_ROUTINEtakes the same branch on host and device;src/commonfrom 24 to 23.The sixth slice removes low-risk stage selection from common validation and finite differences:
nGlobalwhile simulation skips the check;src/commonfrom 23 to 19.The seventh slice moves common global declarations, equation indexing, and GPU declarations to explicit ownership:
num_dims,num_vels,weno_polyn, andmuscl_polyndeclarations for all executables, while preserving simulation case optimization;eqn_idx%alfstage guard with an explicit initialization policy, preserving simulation, pre-process, and post-process behavior;src/commonfrom 19 to 14.The eighth slice makes common MPI transport stage-independent:
src/commonfrom 14 to 9.The ninth slice makes MPI domain decomposition stage-independent and removes its repeated directional policy code:
src/commonfrom 9 to 3.The tenth slice makes common STL model setup available in every executable:
s_instantiate_STL_models;num_stl_modelsandstl_modelsto post-process without adding either variable to its namelist;src/commonfrom 3 to 1.The eleventh slice makes the full common phase-change module available in every executable:
palpha_epsandptgalpha_epsto post-process without adding them to its namelist;src/common.Design choices
Stage identity is no longer used as a proxy for accelerator execution. Code inside accelerator regions selects the device-callable kernel with
MFC_GPU; CPU execution uses the host wrapper, which can also populate optional post-processing diagnostics.The scalar conversion kernel is marked with
GPU_ROUTINEand is shared by CPU and accelerator paths. Device-resident module data retains explicitGPU_DECLARE, enter-data, update, and delete lifecycle operations. Material arrays and viscosity metadata are declared only once, in their common owning module.Some variables must now be declared in all three executables because common code references them, but they should not become accepted inputs in every namelist. The parameter generator therefore separates Fortran declaration targets from namelist targets. Existing user-facing scopes for
avg_state,alt_soundspeed,mixture_err,sigR,viscous, andriemann_solverremain unchanged.This change deliberately does not remove capability guards such as
MFC_GPUor backend-specific GPU macros. It removes stage-based implementation selection.Pre-process currently uses
dx,dy, anddzas scalar minimum spacings, whereas simulation and post-process use those names for width arrays. Passing the grid representation explicitly allows the common boundary implementation to compile for every executable without hiding that semantic mismatch behind another stage macro.That mismatch is resolved in the third slice: scalar minimum spacings now have distinct common names, while per-cell width arrays retain the established
dx,dy, anddznames.MPI I/O now follows the same ownership rule: common code constructs views and binds only the objects supplied by its caller. Paired optional data are checked together, preventing a marker without a descriptor or one QBMM node field without the other. Accelerator ownership is unchanged because these routines set host-side MPI pointer/view metadata and introduce no device-data movement.
Dirichlet behavior is configured when the boundary module is initialized, rather than inferred from the executable name. The policy is device resident because
s_dirichletis called inside accelerator boundary loops.Finite-difference routines now compute into caller-provided storage. This keeps allocation and lifetime with the executable that owns the array and avoids hiding post-process reallocation inside common numerical code.
Generated declarations are now the ownership boundary for generated simulation inputs. The generator emits exactly one
GPU_DECLAREfor each device-resident scalar it declares. Manually declared common state retains direct portability macros in the common module. Capability guards such asMFC_GPU, OpenACC, and OpenMP remain intact; only executable-stage selection is being removed.MPI transport now has one implementation. Executables configure whether all chemistry temperatures need halo exchange and whether RDMA transport is enabled; the common layer owns buffer sizing and selects the corresponding transport path. OpenACC/OpenMP data movement remains expressed through the portability macros.
MPI decomposition owns the Cartesian algorithm, while executables provide only the data and policies that are actually stage-specific. Pre-process supplies mutable domain bounds and requests local-domain adjustment for generated grids; post-process supplies mutable output offsets and requests Silo overlap; simulation disables both policies. The common implementation applies both policies dimensionally, eliminating the three copies of each formula.
STL instantiation remains a single common routine. Post-process now has the declarations needed to compile that routine, but no new accepted inputs and no new call site. Generated declaration targets are therefore the compile-time ownership mechanism, while namelist targets remain the user-facing input contract.
Phase-change follows the same ownership rule: the complete numerical implementation is compiled once in common for all executables, while only the stages that currently need it invoke it. The two solver tolerances are declaration-visible in post-process solely so the common module compiles; their accepted input scope remains pre-process/simulation.
Behavior changes
This is predominantly a refactor, but three changes are not behavior-preserving and are called out deliberately.
Unifying the mixture-conversion kernel means
pre_processandpost_processnow apply thempp_limclamp and volume-fraction renormalization that previously existed only in the simulation path. Formpp_lim = Tthis can change derived quantities wherever the stored volume fractions are slightly out of bounds or do not sum to one. The full golden-file suite, including post-process comparisons, shows no movement in any covered case.Unifying the chemistry halo policy fixes a latent buffer overrun on
master: buffer allocation usedchemistry .and. chem_params%diffusionwhile the exchange itself usedpresent(q_T_sf) .and. chemistryfor pre-process and post-process, so a chemistry run without diffusion packednVar + 1variables into buffers sized forsys_size. Both sites now share one predicate.Consequently
pre_processno longer exchanges the ghost temperature layer when chemistry is enabled without diffusion. Nothing consumes it: elliptic smoothing reads onlyq_prim_vf, ands_compute_T_from_primitivesrecomputes the temperature over the interior immediately afterwards.Type of change
Testing
Built
pre_process,simulation, andpost_processwithout MPI.Built all three targets with MPI.
Built a case-optimized simulation using
examples/1D_euler_convergence/case.py.Ran focused MPI regression cases with post-processing:
8EAC3DA7(1D viscous)9DAC4DDC(two-fluid alternative sound speed)CE9DBA3F(QBMM)B9553426(Lagrangian bubbles)Ran an end-to-end non-MPI
1D_euler_convergenceexample.Ran focused grid and boundary regressions with post-processing:
B49877F3(3D grid stretching)8C7AA13B(2D, two MPI ranks)DB670E50(axisymmetric boundary)3008BA80(periodic immersed-boundary case)Ran focused STL/model regressions:
EA8FA07E(2D IBM STL)E777BE53(3D pre-process ICPP STL)Ran focused MPI I/O regressions:
356A5C50(non-polytropic QBMM with parallel I/O)3008BA80(immersed-boundary marker I/O)Ran a dedicated MPI downsampling smoke case through pre-process, simulation, and post-process; an
11x11x11input was read as the expected3x3x3post-process domain.Ran
986BC1A2(2D Richtmyer--Meshkov with Dirichlet boundary-buffer input) through pre-process, simulation, and post-process with MPI.Built a case-optimized simulation for
examples/2D_richtmyer_meshkov/case.py.Ran
FD891191(two-fluid six-equation model) through pre-process, simulation, and post-process with MPI.Ran
1550B67E(2D chemistry with diffusion disabled) through pre-process, simulation, and post-process with MPI.Re-ran
B9553426(2D Lagrangian bubbles) through all three stages with MPI to exercise ordinary and beta halo transport.Re-ran decomposition regressions through pre-process, simulation, and post-process with MPI:
B49877F3(3D grid stretching)8C7AA13B(2D, two MPI ranks, Silo output)DB670E50(axisymmetric)Ran a two-rank old-grid smoke from
examples/1D_vacuum_restart: generated the original parallel grid, then re-read it withold_grid = Tin a temporary case directory. The temporary restart copy supplied domain bounds required by current validation and disabledold_icso the check exercised grid reuse without requiring a 7000-step restart file.Re-ran STL regressions:
EA8FA07E(2D IBM STL)E777BE53(3D pre-process ICPP STL)Ran phase-change regressions through pre-process, simulation, and post-process with MPI:
E6DBCBD9(1D, model 5, two fluids, six-equation model)02BD9B5E(2D, model 6, three fluids, six-equation model)344FAA70(3D, model 5, two fluids, six-equation model)Ran the parameter generator tests: 93 passed.
Ran the focused Fortran-generator tests after the declaration changes: 48 passed.
Ran the repository pre-commit gate: formatting, spelling, toolchain lint, source lint, documentation references, parameter documentation, and all example validations passed.
Expanded the affected sources for OpenACC and inspected the resulting declarations and data lifecycle. The device conservative-to-primitive path calls the
GPU_ROUTINEkernel directly, and the old stage-specific accelerator routine is absent.Expanded the grid changes for both OpenACC and OpenMP. The host grid population precedes
acc update device(glb_bounds)/omp target update to(glb_bounds), and common grid routines do not take over device-data ownership.Expanded
m_mpi_commonfor all three executables with both OpenACC and OpenMP definitions and inspected the new explicit QBMM, immersed-boundary, and downsampled interfaces. A physical accelerator build is unavailable on this macOS/GNU toolchain.Expanded the Dirichlet change for all three executables with both OpenACC and OpenMP definitions. The policy receives
acc declare/omp declare targetandacc update device/omp target update, and both buffer-copy and extrapolation paths remain in the device routine.Expanded the common global-parameter module for all three executables with both OpenACC and OpenMP definitions. Pre-process/post-process receive only common manual device declarations; simulation receives exactly one generated declaration for each owned GPU scalar.
Expanded a case-optimized simulation and verified that
num_dims,num_vels,weno_polyn, andmuscl_polynremain Fortran parameters with no OpenACC/OpenMP device allocation.Expanded
m_mpi_commonfor all three executables with both OpenACC and OpenMP definitions after transport unification. Every stage receives the same staged and RDMA branches; runtime policy selects one, and the portability macros emit the expected host/device updates or device-address regions.Re-expanded
m_mpi_commonfor every executable and both accelerator backends after decomposition unification; each expansion contains the same single policy helper and no stage-selection residue.Built a case-optimized MPI simulation for
examples/2D_richtmyer_meshkov/case.pyafter the decomposition interface change.Expanded
m_modelfor all three executables with OpenACC and OpenMP definitions. Every expansion contains exactly one STL-instantiation routine, its existing device updates, and no stage-selection residue.Expanded
m_phase_changefor all three executables with OpenACC and OpenMP definitions. Every expansion contains the same full phase-change solver and accelerator loop annotations, with no stage-selection residue.Verified that
rg -n 'MFC_(PRE_PROCESS|SIMULATION|POST_PROCESS)' src/commonreturns no matches.Merged
master(Remove non-operational hyperelasticity capability #1675, hyperelasticity removal) and resolved eight conflicts;m_hyperelastic.fppis deleted,elasticitybecomeshypoelasticity, andb_size/tensor_size/elasticityGPU declarations are dropped.Ran the full golden-file suite with post-process comparisons enabled (
./mfc.sh test -a) on GNU 13.3 with MPI: 577 tests, no golden-file differences.Built all three targets with NVHPC 25.11 and OpenACC, and ran the suite on NVIDIA A100 hardware: 296 cases from the full run plus a 30-case set targeting every device-facing change (IBM STL, immersed-boundary markers, Lagrangian bubbles, non-polytropic QBMM, cylindrical and axisymmetric grids, phase change,
mpp_lim, viscous, and the boundary-condition sweep). No failures.Ran both chemistry cases on GPU (
1550B67E,C4EB58A8) after building the chemistry variant.Built a case-optimized simulation with NVHPC and OpenACC, and confirmed the generated declarations emit case-optimization parameters as Fortran
parameterconstants with no device storage, while the non-case-optimized branch emits exactly the twenty-twoGPU_DECLAREnames the deleted hand-written block contained.syscheckpasses with MPI and OpenACC device offload on the same machine.Cray and Intel are covered only by CI; neither was exercised locally.
Checklist
No user-facing parameter behavior changed; the declaration/namelist separation preserves the existing accepted inputs for each executable. The numerical changes are limited to those listed under Behavior changes.
See the developer guide for full coding standards.
GPU changes (expand if you modified
src/simulation/)Built with NVHPC 25.11 and OpenACC and run on NVIDIA A100 hardware. The GPU suite covers every device-facing change in this PR and reports no failures; the same cases pass on the CPU build against the same golden files. AMD and OpenMP-offload builds were not exercised.
AI code reviews
Reviews are not retriggered automatically. To request a review, comment on the PR:
@claude full review— Claude full review (also triggers on PR open/reopen/ready)claude-full-review— Claude full review via label